-
Notifications
You must be signed in to change notification settings - Fork 104
Drop support for Julia < 1.10 #504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
==========================================
- Coverage 94.11% 93.97% -0.14%
==========================================
Files 14 14
Lines 2905 2904 -1
==========================================
- Hits 2734 2729 -5
- Misses 171 175 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# In future just use `fastabs` from Base.Math | ||
# https://github.com/JuliaLang/julia/blob/93fb785831dcfcc442f82fab8746f0244c5274ae/base/special/trig.jl#L1057 | ||
if isdefined(Base.Math, :fastabs) | ||
import Base.Math: fastabs | ||
else | ||
fastabs(x::Number) = abs(x) | ||
fastabs(x::Complex) = abs(real(x)) + abs(imag(x)) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since fastabs
is technically not public, maybe it's safer to keep this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... maybe. Codecov was complaining (correctly) that these lines are not covered by tests anymore. So in that sense technically the fallback is not safe either as it's untested when dropping support for Julia < 1.10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the better approach would be to mark Base.Math.fastabs
as public
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but good luck with that 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from making Base.Math.fastabs
public, the only other "correct" approach would be to only define it in SpecialFunctions, ie to only keep the second branch. Then we would neither rely on internals nor on untested code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe that's not too bad, it's two very simple methods, and unlikely to need maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I removed the first branch 🙂
See #503 (comment)